Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sf fixes #37976

Closed
wants to merge 157 commits into from
Closed

sf fixes #37976

wants to merge 157 commits into from

Conversation

mantepse
Copy link
Collaborator

@mantepse mantepse commented May 10, 2024

Fix #37975

Depends on #37033

tscrim and others added 30 commits October 10, 2023 12:21
… of cache may contain variable, add failing test
…s, use 'is' for equality when finding input streams in Stream_uninitialized
S = R.base_ring()[tuple(Z)]
S = PolynomialRing(R.base_ring(), Z)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that using __getitem__ to define a polynomial ring in library code is generally a very bad idea. I found a few other occurrences with a very naive grep, possibly one could find more by inserting a print statement in __getitem__ and running the test suite (and work very hard). @fchapoton, do you agree?

grep -r --include=*.{py,pyx} --color -nH --null -e "ring\[" *
grep -r --include=*.{py,pyx} --color -nH --null -e "R\['" *

Here goes:

algebras/hecke_algebras/cubic_hecke_algebra.py:962:        pol_bas_ring = base_ring['h']
combinat/finite_state_machine_generators.py:1276:            polynomial_left = base_ring[var](left_side.operands()[0])
combinat/finite_state_machine_generators.py:1331:            polynomial_right = base_ring[var](next_function.operands()[0])
dynamics/arithmetic_dynamics/projective_ds.py:5769:                T = base_ring['k']
interfaces/sympy.py:195:    return base_ring[variables]
interfaces/sympy.py:227:    R = base_ring[variables]
matrix/matrix_space.py:1111:            return self.__change_ring[R]
matrix/matrix_space.py:1117:        self.__change_ring[R] = M
matrix/matrix2.pyx:3675:        R = self._base_ring[var]    # polynomial ring over the base ring
modular/quasimodform/ring.py:268:        self.__polynomial_subring = self.__modular_forms_subring[name]
schemes/elliptic_curves/hom_velusqrt.py:888:        R, Z = self._internal_base_ring['Z'].objgen()
schemes/elliptic_curves/hom_velusqrt.py:1134:        S = self._internal_base_ring['x,y']
schemes/elliptic_curves/hom_velusqrt.py:1171:        S = self._internal_base_ring['x']
schemes/elliptic_curves/ell_field.py:1604:            return R['x'].one()
schemes/elliptic_curves/ell_generic.py:3399:                R = RR['x']
schemes/elliptic_curves/hom_velusqrt.py:902:                V = R['V'].gen()
dynamics/arithmetic_dynamics/projective_ds.py4575:                    T = R['t']
dynamics/arithmetic_dynamics/projective_ds.py4932:                            T = R['t']
functions/transcendental.py633:                f = PolynomialRealDense(R['x'], coeffs)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree. It is syntactic sugar. Let’s change all of these in the library.

@mantepse
Copy link
Collaborator Author

mantepse commented May 10, 2024

Should I laugh or cry?

sage: h = SymmetricFunctions(QQ).h()
sage: m = matrix([h([])])
sage: m.column(0)
(h[])
sage: -m.column(0)
({[]: 1})

Update 1: apparently, the last line is equivalent to

sage: v = m.column(0)
sage: v * (-1)
({[]: 1})

sage: coercion_model.get_action(v.parent(), ZZ, operator.mul, v, -1)
Right scalar multiplication by Integer Ring on Ambient free module of rank 1 over the integral domain Symmetric Functions over Integer Ring in the homogeneous basis

but I couldn't find the implementation yet.

Update 2:

sage: mu = coercion_model.get_action(v.parent(), ZZ, operator.mul, v, -1)
sage: mu.op
<built-in function mul>

Copy link

github-actions bot commented May 10, 2024

Documentation preview for this PR (built with commit 04b6abc; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@tscrim
Copy link
Collaborator

tscrim commented May 10, 2024

Can we also separate this from #37033 or is there something explicitly in there that this needs?

@mantepse
Copy link
Collaborator Author

Can we also separate this from #37033 or is there something explicitly in there that this needs?

In principle yes, although my main goal is to fix #37975. So, I thought of this as being a "meta-PR". I would actually like to factor out individual commits fixing separate things, once I know what to do.

I am currently stuck with the negation issue mentioned above, and just asked for help on sage-devel.

@mantepse
Copy link
Collaborator Author

With the following experimental change, I am getting a little further.

diff --git a/src/sage/modules/free_module_element.pyx b/src/sage/modules/free_module_element.pyx
index c93c71f195e..c885f1e82c7 100644
--- a/src/sage/modules/free_module_element.pyx
+++ b/src/sage/modules/free_module_element.pyx
@@ -4967,7 +4968,8 @@ cdef class FreeModuleElement_generic_sparse(FreeModuleElement):
         cdef dict v = {}
         if right:
             for i, a in self._entries.iteritems():
-                prod = (<RingElement>a)._mul_(right)
+                prod = a._mul_(right)
                 if prod:
                     v[i] = prod
         return self._new_c(v)

@mantepse
Copy link
Collaborator Author

So, here is where I am now failing:

sage: h = SymmetricFunctions(QQ).h()
sage: H = h.fraction_field()
sage: R.<t, u> = H[]
sage: P.<a> = InfinitePolynomialRing(H)
sage: PF = P.fraction_field()
sage: PF(a[0]) * R(0)
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
File ~/sage/src/sage/structure/coerce.pyx:1219, in sage.structure.coerce.CoercionModel.bin_op()
   1218 try:
-> 1219     action = self._action_maps.get(xp, yp, op)
   1220 except KeyError:

File ~/sage/src/sage/structure/coerce_dict.pyx:1326, in sage.structure.coerce_dict.TripleDict.get()
   1325 if not valid(cursor.key_id1):
-> 1326     raise KeyError((k1, k2, k3))
   1327 value = <object>cursor.value

KeyError: (Fraction Field of Infinite polynomial ring in a over Fraction Field of Symmetric Functions over Rational Field in the homogeneous basis, Multivariate Polynomial Ring in t, u over Fraction Field of Symmetric Functions over Rational Field in the homogeneous basis, <built-in function mul>)

During handling of the above exception, another exception occurred:

KeyError                                  Traceback (most recent call last)
File ~/sage/src/sage/structure/category_object.pyx:855, in sage.structure.category_object.CategoryObject.getattr_from_category()
    854 try:
--> 855     return self._cached_methods[name]
    856 except KeyError:

KeyError: 'ngens'

During handling of the above exception, another exception occurred:

AttributeError                            Traceback (most recent call last)
Cell In[25], line 1
----> 1 PF(a[Integer(0)]) * R(Integer(0))

File ~/sage/src/sage/structure/element.pyx:1506, in sage.structure.element.Element.__mul__()
   1504     return (<Element>left)._mul_(right)
   1505 if BOTH_ARE_ELEMENT(cl):
-> 1506     return coercion_model.bin_op(left, right, mul)
   1507 
   1508 cdef long value

File ~/sage/src/sage/structure/coerce.pyx:1221, in sage.structure.coerce.CoercionModel.bin_op()
   1219     action = self._action_maps.get(xp, yp, op)
   1220 except KeyError:
-> 1221     action = self.get_action(xp, yp, op, x, y)
   1222 if action is not None:
   1223     if (<Action>action)._is_left:

File ~/sage/src/sage/structure/coerce.pyx:1759, in sage.structure.coerce.CoercionModel.get_action()
   1757 except KeyError:
   1758     pass
-> 1759 action = self.discover_action(R, S, op, r, s)
   1760 action = self.verify_action(action, R, S, op)
   1761 self._action_maps.set(R, S, op, action)

File ~/sage/src/sage/structure/coerce.pyx:1900, in sage.structure.coerce.CoercionModel.discover_action()
   1898 """
   1899 if isinstance(R, Parent):
-> 1900     action = (<Parent>R).get_action(S, op, True, r, s)
   1901     if action is not None:
   1902         return action

File ~/sage/src/sage/structure/parent.pyx:2587, in sage.structure.parent.Parent.get_action()
   2585 action = self._get_action_(S, op, self_on_left)
   2586 if action is None:
-> 2587     action = self.discover_action(S, op, self_on_left, self_el, S_el)
   2588 
   2589 if action is not None:

File ~/sage/src/sage/structure/parent.pyx:2695, in sage.structure.parent.Parent.discover_action()
   2693 # detect actions defined by _rmul_, _lmul_, _act_on_, and _acted_upon_ methods
   2694 from sage.structure.coerce_actions import detect_element_action
-> 2695 action = detect_element_action(self, S, self_on_left, self_el, S_el)
   2696 if action is not None:
   2697     return action

File ~/sage/src/sage/structure/coerce_actions.pyx:223, in sage.structure.coerce_actions.detect_element_action()
    221 if isinstance(x, ModuleElement) and isinstance(y, Element):
    222     try:
--> 223         return (RightModuleAction if X_on_left else LeftModuleAction)(Y, X, y, x)
    224     except CoercionException as msg:
    225         _record_exception()

File ~/sage/src/sage/structure/coerce_actions.pyx:388, in sage.structure.coerce_actions.ModuleAction.__init__()
    386 if not isinstance(g, Element) or not isinstance(a, ModuleElement):
    387     raise CoercionException("not an Element acting on a ModuleElement")
--> 388 res = self.act(g, a)
    389 if parent(res) is not the_set:
    390     # In particular we will raise an error if res is None

File ~/sage/src/sage/categories/action.pyx:235, in sage.categories.action.Action.act()
    233         5*x
    234     """
--> 235     return self._act_convert(g, x)
    236 
    237 def __invert__(self):

File ~/sage/src/sage/categories/action.pyx:191, in sage.categories.action.Action._act_convert()
    189     if parent(x) is not U:
    190         x = U(x)
--> 191     return self._act_(g, x)
    192 
    193 cpdef _act_(self, g, x):

File ~/sage/src/sage/structure/coerce_actions.pyx:673, in sage.structure.coerce_actions.RightModuleAction._act_()
    671     g = <Element?>self.connecting._call_(g)
    672 if self.extended_base is not None:
--> 673     a = <ModuleElement?>self.extended_base(a)
    674 return (<ModuleElement>a)._lmul_(<Element>g)  # a * g
    675 

File ~/sage/src/sage/structure/parent.pyx:901, in sage.structure.parent.Parent.__call__()
    899 if mor is not None:
    900     if no_extra_args:
--> 901         return mor._call_(x)
    902     else:
    903         return mor._call_with_args(x, args, kwds)

File ~/sage/src/sage/structure/coerce_maps.pyx:443, in sage.structure.coerce_maps.CallableConvertMap._call_()
    441         print(self._func)
    442         print(C)
--> 443     raise
    444 if y is None:
    445     raise RuntimeError("BUG in coercion model: {} returned None".format(self._func))

File ~/sage/src/sage/structure/coerce_maps.pyx:438, in sage.structure.coerce_maps.CallableConvertMap._call_()
    436         y = self._func(C, x)
    437     else:
--> 438         y = self._func(x)
    439 except Exception:
    440     if print_warnings:

File ~/sage/src/sage/rings/fraction_field.py:339, in FractionField_generic._coerce_map_from_.<locals>.wrapper(x)
    338 def wrapper(x):
--> 339     return self._element_class(self, x.numerator(), x.denominator())

File ~/sage/src/sage/rings/fraction_field_element.pyx:112, in sage.rings.fraction_field_element.FractionFieldElement.__init__()
    110 FieldElement.__init__(self, parent)
    111 if coerce:
--> 112     self._numerator   = parent.ring()(numerator)
    113     self._denominator = parent.ring()(denominator)
    114 else:

File ~/sage/src/sage/structure/parent.pyx:901, in sage.structure.parent.Parent.__call__()
    899 if mor is not None:
    900     if no_extra_args:
--> 901         return mor._call_(x)
    902     else:
    903         return mor._call_with_args(x, args, kwds)

File ~/sage/src/sage/structure/coerce_maps.pyx:163, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_()
    161             print(type(C), C)
    162             print(type(C._element_constructor), C._element_constructor)
--> 163         raise
    164 
    165 cpdef Element _call_with_args(self, x, args=(), kwds={}):

File ~/sage/src/sage/structure/coerce_maps.pyx:158, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_()
    156 cdef Parent C = self._codomain
    157 try:
--> 158     return C._element_constructor(x)
    159 except Exception:
    160     if print_warnings:

File ~/sage/src/sage/rings/polynomial/infinite_polynomial_ring.py:997, in InfinitePolynomialRing_sparse._element_constructor_(self, x)
    994 # By now, x or self._P are not libsingular. Since MPolynomialRing_polydict
    995 # is too buggy, we use string evaluation
    996 try:
--> 997     return sage_eval(repr(x), self.gens_dict())
    998 except (ValueError, TypeError, NameError):
    999     raise ValueError("cannot convert {} into an element of {} - no conversion into underlying polynomial ring".format(x, self))

File ~/sage/src/sage/misc/cachefunc.pyx:2329, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__()
   2327 if self.cache is None:
   2328     f = self.f
-> 2329     self.cache = f(self._instance)
   2330 return self.cache
   2331 

File ~/sage/src/sage/rings/polynomial/infinite_polynomial_ring.py:1287, in InfinitePolynomialRing_sparse.gens_dict(self)
   1272 @cached_method
   1273 def gens_dict(self):
   1274     """
   1275     Return a dictionary-like object containing the infinitely many
   1276     ``{var_name:variable}`` pairs.
   (...)
   1285         a_5
   1286     """
-> 1287     return GenDictWithBasering(self, InfiniteGenDict(self.gens()))

File ~/sage/src/sage/rings/polynomial/infinite_polynomial_ring.py:560, in GenDictWithBasering.__init__(self, parent, start)
    558 while hasattr(P, 'base_ring') and (P.base_ring() is not P):
    559     P = P.base_ring()
--> 560     D = P.gens_dict()
    561     if isinstance(D, GenDictWithBasering):
    562         self._D.extend(D._D)

File ~/sage/src/sage/structure/category_object.pyx:289, in sage.structure.category_object.CategoryObject.gens_dict()
    287 """
    288 if copy:
--> 289     return dict(self.__gens_dict())
    290 else:
    291     return self.__gens_dict()

File ~/sage/src/sage/misc/cachefunc.pyx:2329, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__()
   2327 if self.cache is None:
   2328     f = self.f
-> 2329     self.cache = f(self._instance)
   2330 return self.cache
   2331 

File ~/sage/src/sage/structure/category_object.pyx:263, in sage.structure.category_object.CategoryObject._CategoryObject__gens_dict()
    261 def __gens_dict(self):
    262     cdef dict v = {}
--> 263     for x in self._defining_names():
    264         v[str(x)] = x
    265     return v

File ~/sage/src/sage/misc/cachefunc.pyx:2329, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__()
   2327 if self.cache is None:
   2328     f = self.f
-> 2329     self.cache = f(self._instance)
   2330 return self.cache
   2331 

File ~/sage/src/sage/structure/category_object.pyx:415, in sage.structure.category_object.CategoryObject._defining_names()
    413         ((1/2, 0, 0), (0, 1, 0))
    414     """
--> 415     return self.gens()
    416 
    417 #################################################################################################

File ~/sage/src/sage/structure/parent_gens.pyx:119, in sage.structure.parent_gens.ParentWithGens.gens()
    117 if self._gens is not None:
    118     return self._gens
--> 119 self._gens = tuple(self.gen(i) for i in range(self.ngens()))
    120 return self._gens
    121 

File ~/sage/src/sage/rings/fraction_field.py:814, in FractionField_generic.ngens(self)
    802 def ngens(self):
    803     """
    804     This is the same as for the parent object.
    805 
   (...)
    812         10
    813     """
--> 814     return self._R.ngens()

File ~/sage/src/sage/structure/category_object.pyx:849, in sage.structure.category_object.CategoryObject.__getattr__()
    847         AttributeError: 'PrimeNumbers_with_category' object has no attribute 'sadfasdf'...
    848     """
--> 849     return self.getattr_from_category(name)
    850 
    851 cdef getattr_from_category(self, name):

File ~/sage/src/sage/structure/category_object.pyx:864, in sage.structure.category_object.CategoryObject.getattr_from_category()
    862     cls = self._category.parent_class
    863 
--> 864 attr = getattr_from_other_class(self, cls, name)
    865 self._cached_methods[name] = attr
    866 return attr

File ~/sage/src/sage/cpython/getattr.pyx:357, in sage.cpython.getattr.getattr_from_other_class()
    355     dummy_error_message.cls = type(self)
    356     dummy_error_message.name = name
--> 357     raise AttributeError(dummy_error_message)
    358 cdef PyObject* attr = instance_getattr(cls, name)
    359 if attr is NULL:

AttributeError: 'SymmetricFunctionAlgebra_homogeneous_with_category' object has no attribute 'ngens'

@nbruin
Copy link
Contributor

nbruin commented May 10, 2024

Update 1: apparently, the last line is equivalent to

sage: v = m.column(0)
sage: v * (-1)
({[]: 1})

sage: coercion_model.get_action(v.parent(), ZZ, operator.mul, v, -1)

Note that (-1)*v works just fine, so there is a difference between the left action and the right action:

sage: mu_l = coercion_model.get_action(ZZ,v.parent(),operator.mul,-1,v)
sage: mu_r = coercion_model.get_action(v.parent(),ZZ,operator.mul,v,-1)

where mu_l(-1,v) does what you want and mu_r(v,-1) does not. The operator mu.op is operator.mul in both cases. I think that's just a marker that this action what discovered from a multiplicative action. I don't think it indicates something significant here.

If -v gets translated to right multiplication by -1 on a module, I think there could be an issue: if the module is a left module only, it doesn't really make sense. "-1" will always be in the center, so there's no harm in making it a both-sided module over the center, but that's a non-trivial amount of mathematics to involve! So it may be worth checking if v ends up living in something that is primarily a left-module and if so, how - becomes expressed as a right-action by -1.

If the parent of v really is a two-sided module, something goes wrong in the action discovery there.

@mantepse
Copy link
Collaborator Author

The last commit makes things work in the specific doctest (labelled Dyck paths Frobenius character), but I am not sure whether this is (even in principle) the correct fix.

Also, I think we should be more careful with respect to empty sums in stream.py. I would favour passing the desired ring for the elements of the stream, rather than using ZZ if nothing else is known.

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 7, 2024

superseded by #38429

@mantepse mantepse closed this Oct 7, 2024
@mantepse mantepse deleted the sf-fixes branch October 7, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make SymmetricFunctions work with define_implicitly
3 participants